Refactors the socket broadcast client #329
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit refactors the socket broadcast client. It switches out the underlying queue to a circular buffer, that way the memory pressure of the broadcast client remains constant without needing to do work at sporadic intervals. The new structure should hopefully also make it easier for someon to reason about the websocket layer. Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
koonpeng
requested changes
Mar 8, 2024
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
|
@aaronchongth, @koonpeng this is good to review again. I've added some integration tests. |
Signed-off-by: Arjo Chakravarty <[email protected]>
aaronchongth
previously approved these changes
Mar 22, 2024
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
aaronchongth
previously approved these changes
Apr 1, 2024
koonpeng
requested changes
Apr 5, 2024
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
rmf_websocket/src/rmf_websocket/client/ClientWebSocketEndpoint.cpp
Outdated
Show resolved
Hide resolved
Comment on lines
114
to
117
| // This is a horrible piece of code and defeats | ||
| // the purpose of the tests. But unfortunately websocketpp | ||
| // does not correctly return if a send was successful or not. | ||
| // Thus packets may be lost. |
There was a problem hiding this comment.
For this test, we can use a promise to wait until the reconnect happens before sending the 2nd message. Something like
const std::promise<void> on_init_promise;
auto init_function(std::promise<void>& promise) {
...
promise.set_value();
}
REQUIRE_NO_THROW(on_init_promise.get_future().wait_for(1s));
// send 2nd messageTechnically we can use this for test_client.cpp as well, but this requires the use of the reconnect init function, which kind of makes the 2 tests overlap.
There was a problem hiding this comment.
After much hammering... I'm sorry to say there still are corner cases... It seems that sometimes on my machine the reconnection does not guarantee it stays... reconnected. Im trying to figure out if its abug in the logic or something else.
koonpeng
previously approved these changes
Apr 5, 2024
koonpeng
previously approved these changes
Apr 8, 2024
|
There is a minor style issue which is making the non-asan test failing. |
koonpeng
previously approved these changes
Apr 8, 2024
aaronchongth
previously approved these changes
Apr 8, 2024
arjo129
force-pushed
the
arjo/fix/refactor_rmf_websocket
branch
from
98a29ee
to
292a8a6
Compare
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Tests flaky even after improvements. Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Thanks @koonpeng for spotting the timing issue. It seems you were right about the server not being cleanly closed. This commit fixes that by cleanly closing the server in the test. Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
arjo129
force-pushed
the
arjo/fix/refactor_rmf_websocket
branch
from
292a8a6
to
6ae8075
Compare
koonpeng
approved these changes
Apr 8, 2024
arjo129
added a commit
that referenced
this pull request
Jun 7, 2024
* Refactors the socket broadcast client This commit refactors the socket broadcast client. It switches out the underlying queue to a circular buffer, that way the memory pressure of the broadcast client remains constant without needing to do work at sporadic intervals. The new structure should hopefully also make it easier for someon to reason about the websocket layer. Signed-off-by: Arjo Chakravarty <[email protected]> * ROS-ify logging. Signed-off-by: Arjo Chakravarty <[email protected]> * Remove unessecary dependency Signed-off-by: Arjo Chakravarty <[email protected]> * Add lock Signed-off-by: Arjo Chakravarty <[email protected]> * Header guard Signed-off-by: Arjo Chakravarty <[email protected]> * Use enum for status Signed-off-by: Arjo Chakravarty <[email protected]> * Const reference to prevent copy Signed-off-by: Arjo Chakravarty <[email protected]> * Remove redundant include Signed-off-by: Arjo Chakravarty <[email protected]> * Fix move Signed-off-by: Arjo Chakravarty <[email protected]> * Move everything to one thread. Signed-off-by: Arjo Chakravarty <[email protected]> * Fixed "deadlock" Signed-off-by: Arjo Chakravarty <[email protected]> * Clean up reconnection logic Signed-off-by: Arjo Chakravarty <[email protected]> * Delete move and copy constructors. Signed-off-by: Arjo Chakravarty <[email protected]> * Remove FastAPI file Signed-off-by: Arjo Chakravarty <[email protected]> * More cleanups Signed-off-by: Arjo Chakravarty <[email protected]> * Add unit tests and fix a bunch of corner cases. Signed-off-by: Arjo Chakravarty <[email protected]> * Style Signed-off-by: Arjo Chakravarty <[email protected]> * Address feedback Signed-off-by: Arjo Chakravarty <[email protected]> * Address feedback Signed-off-by: Arjo Chakravarty <[email protected]> * Address logging feedback Signed-off-by: Arjo Chakravarty <[email protected]> * Address feedback Signed-off-by: Arjo Chakravarty <[email protected]> * Style Signed-off-by: Arjo Chakravarty <[email protected]> * Horrible hack to make tests always pass. Signed-off-by: Arjo Chakravarty <[email protected]> * Address Feedback Tests flaky even after improvements. Signed-off-by: Arjo Chakravarty <[email protected]> * Style Signed-off-by: Arjo Chakravarty <[email protected]> * Fix tests. Thanks @koonpeng for spotting the timing issue. It seems you were right about the server not being cleanly closed. This commit fixes that by cleanly closing the server in the test. Signed-off-by: Arjo Chakravarty <[email protected]> * Address feedback about logging. Signed-off-by: Arjo Chakravarty <[email protected]> * Style Signed-off-by: Arjo Chakravarty <[email protected]> --------- Signed-off-by: Arjo Chakravarty <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit refactors the socket broadcast client. It switches out the underlying queue to a circular buffer, that way the memory pressure of the broadcast client remains constant without needing to do work at sporadic intervals. The new structure should hopefully also make it easier for someone to reason about the websocket layer and help us catch bugs if they exist.
I've also included an example socket client and a test server. The following plot shows memory usage after restarting the server multiple times: